Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add option for strictNumbers. Resolves #1128. #1219

Merged
merged 1 commit into from
Jun 9, 2020

Conversation

issacgerges
Copy link
Contributor

@issacgerges issacgerges commented May 22, 2020

What issue does this pull request resolve?
#1128

What changes did you make?
Added an option for strictNumbers which is used in checkDataType to conditionally check isFinite(date). Added documentation

Is there anything that requires more attention while reviewing?
The current behavior of ajv is that integer will always fail NaN and pass Infinity but number currently doesn't. To respect semver, this will continue to behave the same way, and this new option strictNumbers is off by default.

I'm unfamiliar with some of the internals of ajv and believe that my changes to checkDataType are sufficient. Also was unsure why y'all were using data === data vs isFinite(data) in integer (one will only fail NaN, the other will fail NaN and pos/neg Infinity`)

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
lib/compile/util.js Outdated Show resolved Hide resolved
spec/options/strictNumbers.spec.js Outdated Show resolved Hide resolved
@epoberezkin
Copy link
Member

@issacgerges thank you - added some comments.

@issacgerges
Copy link
Contributor Author

Hey @epoberezkin made the changes you suggested. I ended up leaving isFinite and linked to the spec showing it is ES5 compatible. I think you were thinking of Number.prototype.isFinite not isFinite

@issacgerges
Copy link
Contributor Author

@epoberezkin any other changes you'd like me to make?

var validate = ajv.compile({type: 'integer'});
validate(1).should.equal(true);
validate(NaN).should.equal(false);
validate(Infinity).should.equal(true);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just noticed it, sorry - I think integer should also not allow Infinity with strictNumbers.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think anybody would want to have a separate option and integers are also numbers (i.e. they would not fail validation with type: 'number'). The idea of type: 'integer' is to impose stricter constraint than number, but in this case it becomes somewhat overlapping rather than stricter.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And I would also add non-integer examples to the tests, to make sure nothing was broken with the new option (e.g. 1.1 would pass number but fail integer).

@epoberezkin
Copy link
Member

@epoberezkin any other changes you'd like me to make?

made one small comment about integer with strict numbers - sorry, missed it before.

Almost there.

@issacgerges
Copy link
Contributor Author

@epoberezkin any other changes you'd like me to make?

made one small comment about integer with strict numbers - sorry, missed it before.

Almost there.

no worries @epoberezkin, I believe I should have met all your requests this time. let me know!

@epoberezkin epoberezkin merged commit f3cb494 into ajv-validator:master Jun 9, 2020
@epoberezkin
Copy link
Member

Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

2 participants